Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bot config #38

Conversation

whilefoo
Copy link

@whilefoo whilefoo commented Nov 3, 2023

I still have some issues because of circular dependencies

@0x4007 0x4007 changed the base branch from refactor/general to feat/contributor-incentives-total-scoring November 11, 2023 00:25
@0x4007 0x4007 changed the base branch from feat/contributor-incentives-total-scoring to refactor/general November 11, 2023 00:25
@whilefoo whilefoo marked this pull request as ready for review November 12, 2023 09:25
@whilefoo
Copy link
Author

@pavlovcik can you take a look?

@0x4007
Copy link
Owner

0x4007 commented Nov 12, 2023

If it's stable let's just merge it and I'll continue working on it on my branch

@whilefoo
Copy link
Author

ok lets merge then

@0x4007 0x4007 changed the base branch from refactor/general to feat/contributor-incentives-total-scoring-initialization November 13, 2023 16:59
@0x4007 0x4007 changed the base branch from feat/contributor-incentives-total-scoring-initialization to feat/contributor-incentives-total-scoring November 13, 2023 16:59
// }
return { valid: true, error: null };
// } catch (error) {
// throw console.trace(error);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are errors still logging?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this code is not used anymore, I have to remove it

@@ -33,7 +33,7 @@ export enum GitHubEvent {
export enum UserType {
User = "User",
Bot = "Bot",
// Organization = "Organization",
Organization = "Organization",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this is a hallucination

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it was failing the payload validation

@@ -79,7 +78,7 @@ export const processors: Record<string, Handler> = {
post: [],
},
[GitHubEvent.PUSH_EVENT]: {
pre: [validateConfigChange],
pre: [],
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed?

Copy link
Author

@whilefoo whilefoo Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it needs to execute before loading the config because loading it will fail if it's invalid. It's here now

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have another approach for reimplementation of this feature? The purpose of it as you may recall is to immediately diagnose broken configs. Which can happen regularly with partners as they update them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved it here, it's still working.
Now thinking about it, it might make sense to merge it with loadConfiguration

@@ -63,7 +63,8 @@
"prettier": "^2.7.1",
"probot": "^12.2.4",
"tsx": "^3.12.7",
"yaml": "^2.2.2"
"yaml": "^2.2.2",
"zod": "^3.22.4"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you said you had to remove zod

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my bad

Copy link
Owner

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging anyways but address these comments and make adjustments in a new pull request if necessary. Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't remembering removing that, it might be an accident

@0x4007 0x4007 merged commit d057dc6 into 0x4007:feat/contributor-incentives-total-scoring Nov 13, 2023
3 of 5 checks passed
@0x4007
Copy link
Owner

0x4007 commented Nov 14, 2023

From the Telegram conversation

🖼 https://github.com/ubiquity-whilefoo/ubiquibot-config/blob/main/.github/ubiquibot-config.yml

i cross referenced your org config. its not clear to me how to enable the slash commands

ill dig through the source code in a bit

🖼 let me know how to fix my config

https://github.com/pavlovcik/ubiquibot-config/blob/development/.github/ubiquibot-config.yml

#40 (comment)

chatgpt says my yaml is correct so perhaps your code isnt properly implemented

also it should be throwing errors if my config is wrong so you need to restore that capability

also i dont think you're loading from the ubiquibot-config-default.ts anymore? that seems wrong

i think i found the issue:

i had an almost empty repo config. im pretty sure that becuase i had no properties for commands, it overwrote and set them all to false. of course this must be fixed.

i just deleted the local repo config and now the start command seems to work.

🖼 also trying to understand why wallet is false when its set to true in the config

photo_2023-11-14 15 27 42

https://github.com/pavlovcik/ubiquibot-config/blob/d69bdff0ad050c1cf89b2fe6272c2965b7475671/.github/ubiquibot-config.yml#L57-L73

Copy link

# I do not understand how to respond to that command

@0x4007
Copy link
Owner

0x4007 commented Nov 14, 2023

Some additional points:

  • OpenAI is not initialized and doesn't work. You should test permit generation before submitting fixes.
  • Keys should be in .env or GitHub Secrets. It doesn't make sense to expose the OpenAI key in the config.

Copy link

# I do not understand how to respond to that command

Copy link

# I do not understand how to respond to that command

@whilefoo
Copy link
Author

whilefoo commented Nov 14, 2023

🖼 also trying to understand why wallet is false when its set to true in the config

photo_2023-11-14 15 27 42

https://github.com/pavlovcik/ubiquibot-config/blob/d69bdff0ad050c1cf89b2fe6272c2965b7475671/.github/ubiquibot-config.yml#L57-L73

The problem was that repo and org were validated and defaults were added automatically and after that the lodash merged them so repo defaults overwrote org properties, so the fix is to first merge the configs and then validate and add defaults.

However I'm still not sure if lodash can merge arrays correctly because I don't know how it chooses the key to merge by or it just joins them, so I propose we change it to:

commands:
  start: true
  stop: false

OR

commands:
  start:
    enabled: true
    # ...other future properties
  stop:
    enabled: false
    # ...other future properties

This will also prevent duplicates and Typescript can detect if some are missing.

@0x4007
Copy link
Owner

0x4007 commented Nov 14, 2023

Perhaps we shouldn’t use lodash if it has this limitation. We should aim to make the configuration expressive and partner friendly.

I think it’s more intuitive to simplify further such as

commands: 
   | start
   | stop
   | query

etc. which to my understanding represents an array of strings.

whilefoo added a commit to ubiquity-whilefoo/ubiquibot that referenced this pull request Nov 14, 2023
@whilefoo
Copy link
Author

whilefoo commented Nov 14, 2023

image

It seems it replaces by index.
So we have to find another library that handles this or make our own version

@whilefoo
Copy link
Author

Actually lodash has a mergeWith function that accepts a custom function, I'll try that first

@whilefoo
Copy link
Author

I can make it merge object arrays by a certain property such as name but the problem is that the algorithm needs to be generic thus any array will need to have the same field. Or I can make it that repo arrays completely overwrite the org arrays without trying to merge them.
For string arrays I can just merge them and remove duplicates.

@0x4007
Copy link
Owner

0x4007 commented Nov 14, 2023

Taking a step back, I feel the only part of the configuration that is a headache to deal with / is not super intuitive are the commands being enabled.

Perhaps we can focus on simplifying or taking a different approach related to those?

Otherwise I think that object assign should be pretty much fine for all primitive values. For example, if a key exists on an org and repo config in the same place, we clobber and take the repo config string.

I wonder what structure makes the most sense for commands. I suppose that an array of strings isn't a primitive data type.

This is verbose but what about new root-level property names that are the command names?


Another idea is that we take the opposite approach with our commands and enable all by default BUT have disables on the repository (or even org) level?

In this case we can have that array of strings:

disabled:
   | start
   | stop
   | query

Given that we rarely disable commands, I feel that this might be the right approach.

Not to complicate things further but when I disabled on ubiquibot the other week, I wished that we were able to also leave an automated message for why to efficiently explain to the contributors. That structure may look more like this:

disabled:
   start: A large refactor is happening at https://github.com/ubiquity/ubiquibot/pull/644 please standby until this is completed. 
   query: User privacy is prioritized in this organization.  

@whilefoo
Copy link
Author

If we use disabled then commands disabled in org config can't be enabled in repo config (you can only disable more commands).
Maybe instead of trying to merge we just overwrite the whole array, so if repo doesn't have disabled array it will use the org's array but if it does then it will ignore org's array and just use repo

Another case with this problem are the labels because they are array of objects:

labels:
  time:
    - name: a
    - name: b
  priority:
    - name: a

@0x4007
Copy link
Owner

0x4007 commented Nov 15, 2023

If we use disabled then commands disabled in org config can't be enabled in repo config (you can only disable more commands).

I think this might be okay. At least for our purposes. I can't think of a time where I needed the capability to selectively enable for a repository while the command was disabled on all the others.

Maybe instead of trying to merge we just overwrite the whole array, so if repo doesn't have disabled array it will use the org's array but if it does then it will ignore org's array and just use repo

From the partner's perspective this does not seem intuitive.

The last idea I have for now is to manage this in Supabase with a new command. This is probably reasonable to do if we can't find a good, partner friendly, config schema for these commands.

However I have mixed feelings on it because I think it makes the most sense to be able to manage the repository settings in one place (instead of now potentially three places- two yml and one database)

Copy link

# I do not understand how to respond to that command

Copy link

# I do not understand how to respond to that command

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants